Skip to content

Fix remaining ty static type checker warnings and errors. - #139

Open
roberthdevries wants to merge 1 commit into
wolfSSL:masterfrom
roberthdevries:fix-remaining-ty-warnings
Open

Fix remaining ty static type checker warnings and errors.#139
roberthdevries wants to merge 1 commit into
wolfSSL:masterfrom
roberthdevries:fix-remaining-ty-warnings

Conversation

@roberthdevries

Copy link
Copy Markdown
Contributor

This will also allow to run ty check in the pipeline static checks step.

This will also allow to run ty check in the pipeline static checks step.

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 6 total — 5 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)

Posted findings

  • [High] EccPrivate.encode_key() accepts with_curve but silently ignores itwolfcrypt/ciphers.py:1396-1409
  • [Medium] _Hmac is no longer abstract after removing the _type abstractmethodwolfcrypt/hashes.py:447-503
  • [Medium] New uv run ty check CI gate may never fail the build.github/workflows/python-app.yml:35-38
  • [Low] _type should be annotated ClassVar[int], not intwolfcrypt/hashes.py:460

Findings not tied to a diff line

New ty suppressions use a non-standard ignore [code] spacing

File: tests/test_mldsa.py:235,252; tests/test_mlkem.py:624,633
Function: test_sign_with_seed_bad_type / test_make_key_from_seed_bad_type / test_make_key_with_random_bad_random_type / test_encapsulate_with_random_bad_random_type
Severity: Medium

All four suppressions added by this PR are written as # ty: ignore [invalid-argument-type] with a space between ignore and [. Every one of the roughly fifty pre-existing suppressions in this repository uses the tight form # ty: ignore[code] -- including tests/test_mldsa.py:206 (_ = mldsa_priv.sign_with_seed(message, "") # ty: ignore[invalid-argument-type]), which sits twenty-nine lines above one of the new ones in the same file, and every suppression this same PR adds to wolfcrypt/ciphers.py and wolfcrypt/hkdf.py.

ty's suppression grammar expects the code list to follow ignore directly; a bare # ty: ignore (no bracket immediately after) is the blanket form that suppresses every diagnostic on the line. With the space, ty will most likely either (a) parse these as blanket suppressions and treat [invalid-argument-type] as free-form trailing comment text, or (b) flag them under invalid-ignore-comment. Outcome (a) is the dangerous one: these lines would silently swallow any future unrelated diagnostic rather than just the intended invalid-argument-type, and the narrowing the author clearly intended would be lost without any signal. Outcome (b) produces noise that the new CI gate is supposed to be catching.

Recommendation: Delete the space before [ in all four new suppressions so they match the repository-wide convention and are unambiguously parsed as scoped (not blanket) suppressions.

Referenced code: tests/test_mldsa.py:235,252; tests/test_mlkem.py:624,633 (11 lines)


Skipped findings

  • [Medium] No test covers the newly added with_curve parameter

Review generated by Skoll

Comment thread wolfcrypt/ciphers.py
@@ -1394,7 +1394,7 @@ def decode_key_raw(self, qx: BytesOrStr, qy: BytesOrStr, d: BytesOrStr, curve_id
raise WolfCryptApiError("Key decode error", ret)

@override

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [High] EccPrivate.encode_key() accepts with_curve but silently ignores it

To silence ty's invalid-method-override, the signature of EccPrivate.encode_key was widened from (self) -> bytes to (self, with_curve: bool = True) -> bytes, but the body was not touched. It still calls _lib.wc_EccKeyToDer(self.native_object, key, len(key)). Per the CFFI cdef in scripts/build_ffi.py:1112 (int wc_EccKeyToDer(ecc_key*, byte* output, word32 inLen);), that wolfSSL API has no curve flag at all -- only wc_EccPublicKeyToDer (used by the base EccPublic.encode_key, ciphers.py:1213) takes with_curve. The new parameter is therefore dead: it is accepted, type-checked, documented by its presence in the signature, and then dropped on the floor.

This is a behavioral regression, not just dead code. Before this PR, ecc_priv.encode_key(with_curve=False) raised TypeError: encode_key() takes 1 positional argument but 2 were given -- a loud, immediate failure. After this PR the same call silently returns curve-bearing DER, i.e. exactly the opposite of what the caller asked for. This is the LSP violation ty was warning about, converted from a noisy runtime error into a silent wrong answer.

It is also inconsistent with the rest of this same diff. The four other override mismatches touched here (EccPrivate.decode_key_raw at line 1375, EccPrivate.encode_key_raw at line 1412, Ed25519Private.encode_key at line 1683, Ed448Private.encode_key at line 1892) were all resolved with # ty: ignore[invalid-method-override], deliberately preserving the runtime signature. Only this one method had its signature altered, and it is the one case where doing so…

Fix: Revert the signature to def encode_key(self) -> bytes: and suppress with # ty: ignore[invalid-method-override], matching the four sibling methods fixed the same way in this diff. If the widened signature is intentionally kept for LSP substitutability, the parameter must not be a silent no-op: raise ValueError("with_curve=False is not supported for ECC private keys") when with_curve is falsy, and document the parameter in the docstring.

Comment thread wolfcrypt/hashes.py
def _type(self) -> int: ...

def _hmac_init(self, hmac: int, key: bytes) -> int:
ret = _lib.wc_HmacInit(self._native_object, _ffi.NULL, -2)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] _Hmac is no longer abstract after removing the _type abstractmethod

The diff deletes the @property @abstractmethod def _type(self) -> int: ... declaration and replaces it with a bare class-body annotation _type: int (line 460). A bare annotation creates no attribute and, critically, is not an abstract member -- so it removes the only thing that was keeping _Hmac abstract.

Walking the ABC bookkeeping: _Hash declares _init, _update, _final, _native_size, _native_type, digest_size, and new as abstract. _Hmac supplies concrete overrides or class attributes for every one of them (digest_size = None, _native_type = "Hmac *", _native_size = _ffi.sizeof("Hmac"), _init, _update, _final, new). With _type gone from __abstractmethods__, that set is now empty and _Hmac is instantiable.

Concretely: _Hmac(b"key") previously raised TypeError: Can't instantiate abstract class _Hmac with abstract method _type before any C resource was touched. It now proceeds into __init__, allocates the native object via _ffi.new(self._native_type), and only then dies at self._hmac_init(self._type, key) (line 479) with AttributeError: '_Hmac' object has no attribute '_type'. The same loss of enforcement applies to any future HMAC variant added to this file: forgetting _type = _TYPE_... used to be a class-instantiation-time error and is now a late AttributeError from inside __init__. _Hmac is a documented extension point (docs/mac.rst:25 has .. autoclass:: _Hmac), so this matters beyond internal use.

Fix: Restore an equivalent guard so a missing _type still fails early and legibly. Either add an __init_subclass__ check as shown, or keep _Hmac explicitly abstract (e.g. leave a trivial abstract member) so _Hmac itself cannot be constructed. At minimum, guard __init__ with an explicit check that produces a clear message instead of a raw AttributeError.

@@ -33,7 +33,9 @@ jobs:
- name: Install the project
run: uv sync --dev
- name: Perform static checks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] New uv run ty check CI gate may never fail the build

The PR's stated purpose is to "allow to run ty check in the pipeline static checks step," but the step as written may be a no-op gate. pyproject.toml contains [tool.ty.rules] with all = "warn", which downgrades every ty rule from its default severity to warning level. ty check exits non-zero for error-level diagnostics; warning-level diagnostics require the --error-on-warning flag to affect the exit code. If that is the case here, the step will print diagnostics to the CI log and still exit 0, so the type regressions this PR just spent effort eliminating could silently reappear without turning CI red.

The multi-command run: | block itself is fine -- GitHub Actions uses bash -e {0} on Linux, so a failing uv run ruff check will still abort before ty check.

Fix: Verify the gate actually blocks: temporarily introduce a deliberate type error on the branch and confirm the job goes red. If it does not, either add --error-on-warning to the CI invocation or promote the rules that should be blocking from "warn" to "error" in [tool.ty.rules].

Comment thread wolfcrypt/hashes.py
digest_size = None
_native_type = "Hmac *"
_native_size = _ffi.sizeof("Hmac")
_type: int

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] _type should be annotated ClassVar[int], not int

_type: int declares an instance variable, but every concrete subclass assigns it at class level -- HmacSha._type = _TYPE_SHA (line 533), HmacSha256._type = _TYPE_SHA256 (line 545), HmacSha384 (line 557), HmacSha512 (line 569) -- and wolfcrypt/hkdf.py reads it straight off the class object, never an instance: hash_cls._type at hkdf.py:70, hkdf.py:106, and hkdf.py:137, where hash_cls: type[_Hmac]. ClassVar[int] states the actual contract (a class-level constant selecting the wolfCrypt HMAC type id), makes class-object access unambiguous for checkers, and prevents a subclass from accidentally shadowing it per-instance. Worth doing since mypy is also in the dev dependency group and may treat class-object access to a plain instance annotation less permissively than ty.

Fix: Change the annotation to _type: ClassVar[int] and add ClassVar to the typing import at wolfcrypt/hashes.py:26.

@dgarske dgarske assigned roberthdevries and unassigned dgarske Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants